Fixup several warnings #9009
Fixup several warnings #9009
Conversation
plemarquand
requested review from
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
- Remove unnecessary `try` statements for function calls that do not throw - Remove unnecessary `#expect` checks for values that cannot be `nil`
plemarquand
force-pushed
the
fixup-warnings
branch
from
6625710 to
df13ad4
Compare
|
@swift-ci test |
plemarquand
force-pushed
the
fixup-warnings
branch
from
3e6cd06 to
4855cdd
Compare
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci please test windows |
| @@ -13,6 +13,7 @@ | |||
| import SourceControl | |||
|
|
|||
| import class Basics.AsyncProcess | |||
| import class TSCBasic.Process | |||
There was a problem hiding this comment.
suggestion: can we use AsyncProcess that is located in SwiftPM instead of having to rely on a TSC API?
There was a problem hiding this comment.
Switching from Process.checkNonZeroExit to AsyncProcess.checkNonZeroExit would require many methods to be converted to async. While probably worthwhile, it would would be best addressed in a standalone PR.
| @@ -32,12 +32,12 @@ import Testing | |||
| import func XCTest.XCTFail | |||
| import struct XCTest.XCTSkip | |||
|
|
|||
| import class TSCBasic.Process | |||
There was a problem hiding this comment.
issue: Can we use AsyncProcess located in SwiftPM instead of depending on a TSC API.
There was a problem hiding this comment.
That would require marking the utility method as async and then cascading that across all its usages. Probably worth doing, but in a separate patch
| @@ -721,7 +721,7 @@ final class MiscellaneousTestCase: XCTestCase { | |||
| guard case SwiftPMError.executionFailure(_, _, let stderr) = error else { | |||
| return XCTFail("invalid error \(error)") | |||
| } | |||
| XCTAssert(stderr.contains("error: You don’t have permission"), "expected permissions error. stderr: '\(stderr)'") | |||
| XCTAssert(stderr.contains("error: You don’t have permission") || stderr.contains("invalid access"), "expected permissions error. stderr: '\(stderr)'") | |||
There was a problem hiding this comment.
issue (possibly-blocking): I this is also update a test case. do we know if this new expectation is expected, or whether it's a regression?
There was a problem hiding this comment.
This change is unrelated to my patch, but its because this test is failing on main. I had to fix it to get CI to pass. To verify, make sure you do a swift package update first.
There was a problem hiding this comment.
I understand it's failing on main, but could this mean a regression was introduced somewhere? That is, test is there for a reason and may have actually served its purpose.
There was a problem hiding this comment.
I wasn't able to track down what specifically caused this to change, but this failure stems from the underlying API moving from the Foundation based file system APIs to the tools-support-core FileSystem.
There was a problem hiding this comment.
It would be ideal to understand the root cause and determine whether the is the expected behaviour or a regression, instead of simply updating the test to reflect the current behaviour - which may be a regression.
There was a problem hiding this comment.
Its likely this change: https://github.com/swiftlang/swift-tools-support-core/pull/521/files#diff-4a8cbeddae09f50e3bf71a7b4f098244ee50dad510e6359a24f2678a5bca10d5R506, converting the NSError's produced inside various FileSystem functions into FileSystemErrors.
|
@swift-ci test self hosted |
|
The previous self hosted windows has the following set failures, which appear unrelated to this change I'm re-triggering build. @swift-ci test self hosted windows |
|
@swift-ci test |
|
@swift-ci test windows |
trystatements for function calls that do not throw#expectchecks for values that cannot benil